Skip to content

Conversation

bryanjnelson
Copy link
Contributor

Closes #3559.

@bryanjnelson bryanjnelson force-pushed the 3559-department-id-blacklist branch from 4924f3f to d51a6e4 Compare September 17, 2025 20:50
@Migration('2025-09-17T09:00:00')
export class CreateUsedDeptIdListMigration extends BaseMigration {
async up() {
const filePath = new URL(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarsonF - What's the best way to pass this filePath in instead of hardcoding it? I tried using the constructor, but then I'd have to use a useValue in the provider in the module, or maybe have it as an env var or something. Is there a better way?

@CarsonF
Copy link
Member

CarsonF commented Sep 18, 2025

I probably wouldn't bother with a neo4j migration. It's not like each env has its own data that needs its own manipulation. and this is really only needed in prod.

Also we don't want the full list from this file, since it has IDs of our own projects. We'll need to exclude those.

@bryanjnelson
Copy link
Contributor Author

I probably wouldn't bother with a neo4j migration. It's not like each env has its own data that needs its own manipulation. and this is really only needed in prod.

Also we don't want the full list from this file, since it has IDs of our own projects. We'll need to exclude those.

I wasn't sure about the migration, so that's helpful.

Now about the list, I wasn't thinking we had to account for Cord Ids because of Seth's comment yesterday:

"This is the full historic list of IDs from Finance (minus Cord existing IDs)"

I took that to mean that Cord Ids were already removed from that list. I'll follow-up in Slack on that.

@bryanjnelson bryanjnelson force-pushed the 3559-department-id-blacklist branch from d51a6e4 to 38c4bdf Compare September 18, 2025 15:54
@bryanjnelson bryanjnelson added the 🚫 Don't Merge This pull request or branch should not be merged. Further changes, reviews, or approvals required label Sep 18, 2025
@bryanjnelson bryanjnelson force-pushed the 3559-department-id-blacklist branch from 1301286 to 3881c54 Compare September 18, 2025 19:06
@bryanjnelson bryanjnelson force-pushed the 3559-department-id-blacklist branch from 3881c54 to 470a8ae Compare September 18, 2025 20:20
@bryanjnelson
Copy link
Contributor Author

bryanjnelson commented Sep 18, 2025

@CarsonF - I ran this locally and received the expected results:

[database:results:stats] Query +4s { nodesCreated: 564, propertiesSet: 2820, labelsAdded: 564 }

Could you give this a look before we actually modify Prod? Also, I'm unclear on what to put as the value of createdBy?

(Also, also, we're not going to check this code in as a migration; this is just allowing easy testing and code review.)

// Get blacklisted IDs
.subQuery((sub) =>
sub
.match(node('blacklist', 'BlacklistDepartmentId'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the label ExternalDepartmentId to better represent the positive nature of work done externally, rather than coupling it to the need to exclude/"blacklist" these from our own generation.

)
// Distill to available (excluding both used AND blacklisted)
.with(
'[id in enumerated where not id in used and not id in blacklisted][0] as next',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to merge our own used and the externally used IDs into one list first, before this logic happens.

// Get used IDs
.subQuery((sub) => sub
  .subQuery((sub2) => sub2
    .match([
      node('', 'Project'),
      relation('out', '', 'departmentId', ACTIVE),
      node('deptIdNode', 'Property'),
    ])
    .where({ 'deptIdNode.value': not(isNull()) })
    .return('deptIdNode.value as id')
    .union()
    .match(node('external', 'ExternalDepartmentId'))
    .return('external.departmentId as id'))
  .return(collect('id').as('used'))
)

@bryanjnelson
Copy link
Contributor Author

Thanks @CarsonF. @rdonigian, I'll let you make the changes in the handler and I'll make the changes in the migration script.

@bryanjnelson bryanjnelson force-pushed the 3559-department-id-blacklist branch from 470a8ae to a996c2c Compare September 18, 2025 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚫 Don't Merge This pull request or branch should not be merged. Further changes, reviews, or approvals required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark additional Department ID's as unavailable for assignment
3 participants